Skip to content

Add allow_denser kwarg for direct decompression into denser SparseMatrixCSC#224

Closed
gdalle wants to merge 8 commits intomainfrom
gd/decomp_bigger
Closed

Add allow_denser kwarg for direct decompression into denser SparseMatrixCSC#224
gdalle wants to merge 8 commits intomainfrom
gd/decomp_bigger

Conversation

@gdalle
Copy link
Member

@gdalle gdalle commented Apr 2, 2025

Fix for SciML/OrdinaryDiffEq.jl#2653 (comment)

User-facing changes

  • Add a keyword allow_denser to the constructors GreedyColoringAlgorithm and ConstantColoringAlgorithm
  • Document what this keyword does in the decompress! docstring: it allows for decompression into a A::SparseMatrixCSC with more nonzeros than the sparsity pattern S used for coloring.
  • This flexibility is only implemented when the following conditions both hold:
    • The decompression is :direct
    • The partition is :row or :column

Internal changes

  • Forward allow_denser to the result types
  • In decompress!, implement decompression into a denser matrix by stepping faster through the nonzeros of A than through the nonzeros of S (using an integer shift)
  • Adapt this logic to single-triangle decompression in the symmetric case
  • Modify check_pattern to allow for more nonzeros if allow_denser=true
  • Add a check_pattern verification to bidirectional decompression. Now the places where users could screw up the sparsity pattern and not notice are:
    • single-color decompression
    • decompression by substitution into a triangle
  • Add a lot of tests

@codecov
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (ba7da77) to head (3faf156).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #224   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines         1758      1816   +58     
=========================================
+ Hits          1758      1816   +58     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gdalle
Copy link
Member Author

gdalle commented Apr 2, 2025

Todo:

  • Set it as an option inside the algorithms
  • Fill with zeros inside the loop

@gdalle gdalle changed the title feat: allow full direct decompression into larger SparseMatrixCSC feat: allow_denser kwarg for direct decompression into denser SparseMatrixCSC Apr 3, 2025
@gdalle gdalle marked this pull request as ready for review April 3, 2025 08:45
@gdalle gdalle requested a review from amontoison April 3, 2025 08:45
@gdalle
Copy link
Member Author

gdalle commented Apr 3, 2025

@amontoison moving this from "enabled by default" to "enabled on demand" took a bit more work than expected, and I had to add a lot more tests to be confident. I wouldn't mind another round of review

src/matrices.jl Outdated
B::Union{SparseMatrixCSC,SparsityPatternCSC},
)
return size(A) == size(B) && nnz(A) == nnz(B)
same_pattern(A::AbstractMatrix, S; allow_denser::Bool=false) = true
Copy link
Collaborator

@amontoison amontoison Apr 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rename this function.
compatible_sparsity_pattern or compatible_pattern is probably a better name.

@amontoison
Copy link
Collaborator

amontoison commented Apr 4, 2025

@gdalle
I think it's fine to not add a kw allow_denser after thinking about it.
It introduces additional complexity for the user that we could avoid that if we are careful in decompress!.
We can do something like this by default to protect the users that provides sparse matrices with a sparsity pattern that doesn't match the one used for coloring.

function decompress!(A::SparseMatrixCSC, B::AbstractMatrix, result::ColumnColoringResult)
    (; compressed_indices) = result
    S = result.bg.S2
    nzA = nonzeros(A)
    if nnz(A) == nnz(S)
        for k in eachindex(compressed_indices)
            nzA[k] = B[compressed_indices[k]]
        end
    elseif nnz(A) > nnz(S)
        rvA, rvS = rowvals(A), rowvals(S)
        shift = 0
        for j in axes(S, 2)
            for k in nzrange(S, j)
                i = rvS[k]
                found = false
                while (k + shift) < A.colptr[j+1] && !found
                    if rvA[k + shift] == i
                        found = true
                        nzA[k + shift] = B[compressed_indices[k]]
                    else
                        shift += 1
                        nzA[k + shift] = zero(eltype(A))
                    end
                end
                !found && error("...")
            end
        end
        nzA[(nnz(S) + shift + 1):end] .= zero(eltype(A))
    else
        error("...")
    end
    return A
end

@gdalle
Copy link
Member Author

gdalle commented Apr 4, 2025

Unfortunately I think your version is incorrect: it doesn't account for the fact that we could find a coefficient (j', i) inside A with j' < j, inside a column that is for instance empty in S. In that case, your while !found loop will stop too early.
As discussed, I suggest we keep my version for now and improve later on, since this is just about error messages being nicer.

@gdalle gdalle changed the title feat: allow_denser kwarg for direct decompression into denser SparseMatrixCSC Add allow_denser kwarg for direct decompression into denser SparseMatrixCSC Apr 4, 2025
@gdalle gdalle marked this pull request as draft April 4, 2025 12:39
@gdalle gdalle added the wontfix This will not be worked on label Apr 4, 2025
@gdalle
Copy link
Member Author

gdalle commented Apr 4, 2025

Following the discussion in SciML/OrdinaryDiffEq.jl#2653 (comment), this is put on hold for the time being. Let's not delete the branch in case we need it again.

@gdalle gdalle closed this Apr 7, 2025
@gdalle gdalle deleted the gd/decomp_bigger branch October 13, 2025 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

wontfix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants